[bsp/milkv 仅用于讨论-后续拆分提交]为后续使用ioremap做准备#9120
[bsp/milkv 仅用于讨论-后续拆分提交]为后续使用ioremap做准备#9120heyuanjie87 wants to merge 8 commits intoRT-Thread:masterfrom heyuanjie87:milkv
Conversation
There was a problem hiding this comment.
写法建议修改为如下方式。理由,没有必要赋值两次,定义时直接放 BSS,可以有助于减小 bin 文件 size
diff --git a/bsp/cvitek/drivers/drv_gpio.c b/bsp/cvitek/drivers/drv_gpio.c
index aef6b291d7..be24f5dcc4 100644
--- a/bsp/cvitek/drivers/drv_gpio.c
+++ b/bsp/cvitek/drivers/drv_gpio.c
@@ -53,8 +53,8 @@ rt_inline void dwapb_write32(rt_ubase_t addr, rt_uint32_t value)
HWREG32(addr) = value;
}
-static rt_ubase_t dwapb_gpio_base = DWAPB_GPIOA_BASE;
-static rt_ubase_t dwapb_gpio_base_e = DWAPB_GPIOE_BASE;
+static rt_ubase_t dwapb_gpio_base;
+static rt_ubase_t dwapb_gpio_base_e;
static struct dwapb_event
{
@@ -303,6 +303,9 @@ static void rt_hw_gpio_isr(int irqno, void *param)
int rt_hw_gpio_init(void)
{
+ dwapb_gpio_base = (rt_size_t)rt_ioremap(DWAPB_GPIOA_BASE, DWAPB_GPIO_SIZE);
+ dwapb_gpio_base_e = (rt_size_t)rt_ioremap(DWAPB_GPIOE_BASE, DWAPB_GPIO_SIZE);
+
rt_device_pin_register("gpio", &_dwapb_ops, RT_NULL);
#define INT_INSTALL_GPIO_DEVICE(no) \There was a problem hiding this comment.
drv_ioremap.h 中的逻辑是否可以合并到 ioremap.h 中,即是否存在通用的逻辑,譬如修改 ioremap.h 如下。
不过我这个修改建议影响面可能会比较大,需要 @BernardXiong 熊大评估一下是否可以。
#ifdef RT_USING_SMART
void *rt_ioremap_early(void *paddr, size_t size);
void *rt_ioremap(void *paddr, size_t size);
void *rt_ioremap_nocache(void *paddr, size_t size);
void *rt_ioremap_cached(void *paddr, size_t size);
void *rt_ioremap_wt(void *paddr, size_t size);
void rt_iounmap(volatile void *addr);
extern void *rt_ioremap_start;
extern size_t rt_ioremap_size;
#else
#define rt_ioremap_early
#define rt_ioremap(paddr, size) (paddr)
#define rt_ioremap_nocache(paddr, size) (paddr)
#define rt_ioremap_cached(paddr, size) (paddr)
#define rt_ioremap_wt(paddr, size) (paddr)
#define rt_iounmap(addr)
#endif // RT_USING_SMARTThere was a problem hiding this comment.
arm64 那边非 smart 也会使用 ioremap API。详细参考 DD2.0 的实现。
There was a problem hiding this comment.
arm64 那边非 smart 也会使用 ioremap API。详细参考 DD2.0 的实现。
哦,是不是说 ioremap 的 API 是否使用取决于是否开启了 mmu,即 ARCH_MM_MMU?
There was a problem hiding this comment.
ioremap 确实依赖于 mmu,但是是否启用应该是偏软件层面的选择。如果整体驱动设计上就是基于运行时配置,动态且灵活的,比如 DD2.0 的实现,那么选择 ioremap 就是必然的(因为编译期是不能确定 MMIO 空间的)。否则,就像大多数 rtos 的实现,io 地址访问就是 1:1 线性映射的空间。那确实没必要使用这个功能。
所以是否使用真正的 ioremap 可以单独做一个 Kconfig,由软件来配置。
There was a problem hiding this comment.
arm64 那边非 smart 也会使用 ioremap API。详细参考 DD2.0 的实现。
我目前对 ioremap 的理解可能有些局限,基于这个 pr 要做的事情来看我的理解就是在开启 mmu 后,我们需要将 io 的物理地址映射到虚拟地址才能访问。那么我的疑问是,难道这个不是开启 mmu 后必选的吗?难道还有什么情况下开了 mmu 后我们依然可以直接用物理地址来访问 io?
此外,我对 @polarvid 上面所说的 ”如果整体驱动设计上就是基于运行时配置,动态且灵活的,比如 DD2.0 的实现,那么选择 ioremap 就是必然的“ 这段描述还不太理解,以及 dd2.0 是什么我还不了解,能否再详细说明一下?这里的 ”运行时配置“ 和 io 地址 remap 是什么关系?
Thanks,
There was a problem hiding this comment.
给libcpu的nommu都补充一份ioremap.h是最方便的了
There was a problem hiding this comment.
给libcpu的nommu都补充一份ioremap.h是最方便的了
没看懂什么意思啊 :(
There was a problem hiding this comment.
ioremap 不是创建虚拟地址映射唯一的方法。具体来说内核支持线性映射和非线性映射,ioremap 从属于非线性映射。因为有两种算法,所以才说选择 ioremap 是软件决定的。更具体地说,选择什么方式是设备模型的设计决定的,所以才提到 DD2.0 框架。
至于为什么、是什么,说来话长了……
There was a problem hiding this comment.
可以在支持mmu这类芯片的驱动里都用上ioremap接口,至于咋映射就交给接口决定,情况复杂了驱动里处理起来麻烦
There was a problem hiding this comment.
dd 2.0,指的是device driver v2.0,相比较原device driver而言。主要来说,它尝试使用设备树了。
至于ioremap怎么怎么着的,我也都弄得不太清楚了。对于ioremap,我的建议是能理顺,逻辑上通顺。反逻辑的就很别扭。
"在开启 mmu 后,我们需要将 io 的物理地址映射到虚拟地址才能访问。" 从这个角度来说,是建议当使用了mmu时,就需要有一份ioremap的,即使这份地址映射是 1:1 的。不知道大家对这份语义是否认同
There was a problem hiding this comment.
和上面对 drv_gpio.c 的思路一样,实际上我觉得目前 drv_uart.c 代码中定义全局变量用宏技巧并没有太大优势,建议统一改成类似如下, 不仅可以优化 bin 的 size,可读性也会好点:
#ifdef BSP_USING_UART0
static struct hw_uart_device _uart0_device;
#endif
......
int rt_hw_uart_init(void)
{
....
#ifdef BSP_USING_UART0
pinmux_config(BSP_UART0_RX_PINNAME, UART0_RX, pinname_whitelist_uart0_rx);
pinmux_config(BSP_UART0_TX_PINNAME, UART0_TX, pinname_whitelist_uart0_tx);
BSP_INSTALL_UART_DEVICE(0);
uart->hw_base = (rt_size_t)rt_ioremap(UART0_BASE, 0x10000);
uart->irqno = UART0_IRQ;
#endif
......
}|
|
||
| #if defined(ARCH_ARM) | ||
| extern rt_ubase_t pinmux_base_ioremap(void); | ||
| #define PINMUX_BASE pinmux_base_ioremap() |
There was a problem hiding this comment.
对 PINMUX_BASE 的访问会很多,可以搜一下 bsp/cvitek/ 下,我发现除了涉及 PINMUX_CONFIG 外,在 sdmmc 驱动中,很多寄存器的定义都是相对于 PINMUX_BASE 的,见 bsp/cvitek/drivers/libraries/sdif/dw_sdmmc.h。如果将 PINMUX_BASE 定义为调用 pinmux_base_ioremap(),效率会降低很多。
所以我这里建议:是否可以将 PINMUX_BASE 定义为 pinmux_base 全局变量, 然后在 board 初始化阶段,确保在早于 uart 初始化之前,就把 pinmux 给初始化了。
There was a problem hiding this comment.
PINMUX_BASE这个暂时保留,这次提交主要是想不太多修改原来代码基础上使用ioremap,效率问题可以后面修改
There was a problem hiding this comment.
建议把这个文件重命名为 drv_pinmix.c。因为根据我的理解(主要是参考 linux 那边),pinctrl 和 pinmux 是两个不同的概念,pinctrl 即 pin control 的缩写,主要指的是对管脚设置譬如上拉,下拉等控制行为,而 pinmux 是 pin multiplex 的缩写,即管脚功能复用。而我们这里应该是指管脚复用,不是吗?
另外,在内核主线树外,我和 @flyingcys 在做一些 pinmux 的开发(感兴趣可以参考 https://github.com/flyingcys/rt-thread/pull/21),只是还没有提交主线上来,相关文件名也是取的 drv_pinmux.c, 所以既然这里已经要加这个文件,建议统一。当然主要的理由还是上面那个。
|
另外补充一些 genernal 的 review 意见: 第一个问题:ioremap 这个功能开启根本原因应该不是为了 smart 吧,而是和 mmu 有关对不对,我看 arm64 核上虽然没有开 smart,但是开了 mmu(ARCH_ARM_MMU+ARCH_MM_MMU),所以目前启用了 ioremap。 如果是这样,所以这个 PR 的修改目的就是为 RISC-V 大核将来和 arm64 保持一致,启用 mmu,同时对 RISC-V 小核不开启 mmu 所以保留 io address 一对一映射机制。 如果以上描述正确,建议修改 pr 描述。 第二个问题:我发现提交的 git commit 中的 log 描述也过于简单了。 tilte 应该用简单的一句话描述修改的内容(不用写改了什么文件,改了哪些文件 git 上很容易看出来) 另外类似 pr 上需要描述的修改目的(即类似上面我写的那些话)、修改方法(简单的设计)以及其他描述(平日如测试方法等)都建议写清楚。可以读一下:
其实 commit 中的内容更重要,pr 的文字不会跟着仓库走的,commit 中的文字会一直在,只要 git 仓库在。 所以我建议改进 commit 的文字描述,这个对于 RTT 的长远发展都有好处,强烈建议 @BernardXiong 熊大强调这个,我看了一些 RTT 里的 pr 和 commit 的描述,都写得好简单,这对项目长期发展不是好事情啊。 RTT 中我觉得 commit 可以写中文啊,应该没有说限制 git 里的 commit 消息一定要用英文吧,如果觉得英文写不好就写中文,据我所知,现在老外用翻译软件看中文那是没有问题的,只要我们中文描述写得足够清楚。 第三个问题我觉得这个 PR 最终提交的时候最好拆分一下,分成三部分比较好
拆分后以后万一需要 revert 和 cherry-pick 也方便。 另外,实际上这个 PR 我看只改了 uart 和 gpio 驱动,还有其他驱动其实也是要考虑的,只是这个 PR 感觉不打算做了? 第四个问题和第三个问题有关,提交 PR 的时候我建议不要保留中间的修改历史。目前我看到了 8 个 commit。 在每次提交 PR 之前,应该将中间修改历史 rebase 压缩掉。 github 上重复利用一个 PR 做多次 review 是可以的,但每次 review 之前都要 rebase 压缩中间过程,只保留最近一次修改的 commit 信息。也就是说如果不考虑第三个问题,我们只看到一个 commit,如果按照第三个问题来做,我们每次 review 看到的是 三个 commit。 |
赞同。 建议放到一个单独 issue 或者 discussion 里面。这些代码规范确实很应该有一个标准。 |
|
如果如标题 “[bsp/milkv 仅用于讨论-后续拆分提交]” 可以按照惯例加前缀 [RFC] |
|
刚才看到 #9123,发现本 pr 的改动和 #9123 有不少重复的地方,特别是 所以看起来这个 PR 中至少这部分内容和 #9123 重复了 @heyuanjie87 @polarvid 你们俩要不要同步一下? |
拉取/合并请求描述:(PR description)
[
bsp中建立的一对一映射暂时保留,未使用ioremap的驱动任然可用
为什么提交这份PR (why to submit this PR)
原驱动中只支持了arm平台的ioremap,为后续milkv运行smart做准备
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
milkv
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality:
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up